Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sdk-vpp#314] Add SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms #110

Merged

Conversation

Bolodya1997
Copy link

Description

Adds SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms.

Actually moves it from sdk-sriov to more relevant place.
https://github.com/networkservicemesh/sdk-sriov/blob/9a5f775aa79d492447aa58867b0a3218be78ff48/pkg/networkservice/common/resourcepool/common.go#L35-L36

Issue link

Needed for networkservicemesh/sdk-vpp#314.

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@edwarnicke
Copy link
Member

Question... the trick you are using here for SRIOVTokenID ... it generalizable to other non-SRIOV kinds of devices we may need to insert? I'm curious, because if it is we may want to pick more generic naming.

To be specific, what I'm thinking is: there's a next gen thing coming down the line called SIOV (not SRIOV)... could we have a DeviceToken that is used for both SRIOV and SIOV (and whatever comes next after SIOV) or do we need to know that the Token is specific to SRIOV?

@Bolodya1997 Bolodya1997 force-pushed the sdk-vpp#314/token-id branch from 0cca414 to 1f5e958 Compare July 30, 2021 14:06
@Bolodya1997
Copy link
Author

To be specific, what I'm thinking is: there's a next gen thing coming down the line called SIOV (not SRIOV)... could we have a DeviceToken that is used for both SRIOV and SIOV (and whatever comes next after SIOV) or do we need to know that the Token is specific to SRIOV?

Client/Endpoint uses SR-IOV token to reserve some VF on startup and then start using it after the Request. I suppose that introducing different type of devices may also contain some logic like this.
So for me it looks like having for such case different types of device IDs can make development and debugging easier.

@@ -46,6 +48,10 @@ const (
// PCIAddressKey - PCI address of the device for the SR-IOV supported mechanisms
PCIAddressKey = "pciAddress"

// SRIOVTokenIDKey - Client/Endpoint SR-IOV token ID
// nolint:gosec
SRIOVTokenIDKey = "sriovTokenID"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to name this something more generic like 'DeviceToken' ... it seems like the trick you are using here could be used more broadly than SRIOV... do I have that right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually can do this, but it can possibly lead to the following behavior if there are not only SR-IOV tokens:

  1. Client requests kernel interface with AAA device token.
  2. Forwarder tries this token as a SR-IOV token - fails.
  3. Fails back to mechanism selection.
  4. Forwarder tries this token as a ... token - fails.
  5. ...
  6. Forwarder establishes the connection.
  7. Client makes refresh request.
  8. Forwarder tries this token as a SR-IOV token - fails.
  9. ...

So we can make a DeviceToken field instead of SRIOVDeviceToken one, but we may possibly need to add some IsDeviceTokenSRIOV in such case to avoid this confusion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to TokenIDKey.

Vladimir Popov added 2 commits September 3, 2021 13:26
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@Bolodya1997 Bolodya1997 marked this pull request as ready for review September 3, 2021 11:38
@@ -33,6 +35,9 @@ const (
// PCIAddressKey - device PCI address property key
PCIAddressKey = common.PCIAddressKey

// TokenIDKey is a token ID property key
TokenIDKey = common.TokenIDKey
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the Token we are getting from DevicePlugin in an ENV variable? If so, could we call it DeviceToken or DeviceTokenID?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure. Fixed.

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>
@edwarnicke edwarnicke merged commit 9a36433 into networkservicemesh:main Sep 7, 2021
nsmbot pushed a commit to networkservicemesh/sdk that referenced this pull request Sep 7, 2021
…i@main

PR link: networkservicemesh/api#110

Commit: 9a36433
Author: Vladimir Popov
Date: 2021-09-08 02:48:27 +0700
Message:
  - [sdk-vpp#314] Add SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms (#110)
* Add SR-IOV token ID mechanism parameter for kernel, VFIO mechanisms

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Renam SR-IOV token ID to just token ID

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

* Rename token ID to device token ID

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants